The issue mentioned two queries.
This tests the first one: SELECT a, b FROM (VALUES ($1, $2)) AS t(a, b)
The second one is verified in the above test: SELECT $1, $2

I use LogicalPlanBuilder::values to recompute the schema for LogicalPlan::Values
causing this line to fail the test from #12339.

Since we allow values without columns, I think this line should be removed.

This is the main change, the new schema needs new_field's data type.

What about other attributes?

I tried to apply new_field's nullability, but the test in insert.slt:305 failed,
because its schema require NOT NULL when the new_field is nullable.

CREATE TABLE table_without_values(field1 BIGINT NOT NULL, field2 BIGINT NULL);

-- statement error Invalid argument error: Column 'column1' is declared as non-nullable but contains null values
insert into table_without_values values(NULL, 300);

So the new schema shouldn't use new_field's nullability.
However, I wasn't quite sure whether other attributes (i.e. metadata) need to be populated.

I'm still trying to wrap my head around what's happening here. From what I can tell, there are two things to consider here:

1. When recomputing new_plan, we lose the column names on the old plan (self) so this is a way of keeping that
2. If we ignore the nullability from self and go with new_plan nullability that also runs into other issue? (Though from what I see this may still cause an error, just later in the pipeline/has a different message)

Am I correct in my understanding here?

I wonder for point 1 if we can do this in a nicer way, perhaps by introducing something like LogicalPlanBuilder::values_with_names to preserve the column names from the start instead of amending after the fact.

For point 2 I'm not sure, perhaps an explicit check upfront is better 🤔

Thank you for looking at this

Am I correct in my understanding here?

Correct.
Allow me to further explain why we need self and why we don't need the whole self.

We need self:

• Point 1: new plan uses different name, this is clearly an issue.
• Point 2: new plan uses different nullability, this allows
  INSERT INTO non_null_table VALUES (NULL) to pass (even though it shouldn't),
  because non-nullable self schema is replaced with nullable new plan schema.

We don't need the whole self:
Let use the sql in this issue SELECT a, b FROM (VALUES ($1, $2)) AS t(a, b) as reference.
After replacing params:

• self (t(a,b)) doesn't know about a and b data types.
• new plan (VALUES ($1, $2)) knows about the data types, but doesn't know about a and b.

So, I was trying to apply new data types while keeping everything in self intact.
It was basically just these two lines, but since I couldn't find something similar to
merge_data_type, I ended up with the whole schema migration.

LogicalPlan::Values(Values { schema, values }) => {
+               let new_plan = // compute new plan here
+               schema.merge_data_type(new_plan.schema());
                Ok(LogicalPlan::Values(Values { schema, values }))

I created a new schema here because I didn't know how to modify fields in a schema.

Please tell me if we have APIs for modifying schema's fields (then some clone can be avoided).

This needs to computed even though plan isn't transformed.
Otherwise changes in a child's schema cannot be populated to
its ancestors.

Without the code change, this line would be:

        Projection: t.a, t.b, t.c [a:Int32;N, b:Null;N, c:Int64;N]